Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unneeded error check in ui tests #48562

Closed

Conversation

GuillaumeGomez
Copy link
Member

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 26, 2018
@GuillaumeGomez
Copy link
Member Author

And it also solves the double compilation issue.

@Mark-Simulacrum
Copy link
Member

I don't feel qualified to understand the check we're removing and why it's not necessary.

r? @petrochenkov

@estebank
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Feb 27, 2018

📌 Commit 3aca663 has been approved by estebank

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2018
@petrochenkov
Copy link
Contributor

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 27, 2018
@petrochenkov
Copy link
Contributor

You can't remove this, UI tests should check ERROR/WARN/etc annotations.
See #46116 and linked issues for the background.

@GuillaumeGomez
Copy link
Member Author

Even with the background, I don't understand its purpose. For this we typically had compile-fail tests. What's the point to check the compiler errors if you compare outputs? If an error/warning/help/whatever isn't thrown out by the compiler, the test will fail.

The other solution which I quickly started would be to actually parse the compiler stderr. And I can assure you that this is not gonna be fun or efficient...

@estebank
Copy link
Contributor

I thought that ui tests were using the rendered field in the json output...

@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 27, 2018
@petrochenkov
Copy link
Contributor

petrochenkov commented Feb 27, 2018

@GuillaumeGomez

Even with the background, I don't understand its purpose.

TLDR: UI testing alone is very unreliable.
Suppose you have a test with N lines that should report an error and M lines that should not report an error. How do you ensure this by having only the auto-generated .stderr file? "Manually" looking through two files trying to find corresponding lines is not reliable, it's very easy to miss something, it's like separating asserts from run-pass tests into separate files. ERROR/WARN annotations must be inline and also not auto-generated.

For this we typically had compile-fail tests.

The plan so far is to gradually migrate them to UI to consistently detect changes in secondary stuff like spans in addition to ERROR annotations (#44844).

@estebank

I thought that ui tests were using the rendered field in the json output...

This was the case until #48337 got merged.

@petrochenkov
Copy link
Contributor

petrochenkov commented Feb 27, 2018

@GuillaumeGomez
I see two ways to resolve #48550:

@petrochenkov petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 27, 2018
@GuillaumeGomez
Copy link
Member Author

Then I'll just go for the --error-format=human output checking. I had someone working in most cases so I think it's doable.

@GuillaumeGomez GuillaumeGomez mentioned this pull request Feb 28, 2018
@petrochenkov
Copy link
Contributor

Closing in favor of #48684

@GuillaumeGomez GuillaumeGomez deleted the remove-useless-check branch March 6, 2018 09:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants